-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for json path syntax #54
Conversation
WalkthroughThe changes introduce support for JSON Path syntax, enhancing the functionality of the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant JsonTemplateLexer
participant JsonTemplateParser
participant JsonTemplateTranslator
participant JsonTemplateReverseTranslator
User->>JsonTemplateLexer: Input JSON Path
JsonTemplateLexer->>JsonTemplateParser: Tokenize Path
JsonTemplateParser->>JsonTemplateTranslator: Parse and Translate Path
JsonTemplateTranslator->>JsonTemplateReverseTranslator: Generate String Representation
JsonTemplateReverseTranslator->>User: Output Translated Path
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success158 tests passing in 3 suites. Report generated by 🧪jest coverage report action from 84aaabf |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
===========================================
- Coverage 100.00% 99.95% -0.05%
===========================================
Files 10 14 +4
Lines 3078 4391 +1313
Branches 726 1047 +321
===========================================
+ Hits 3078 4389 +1311
- Misses 0 1 +1
- Partials 0 1 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- readme.md (2 hunks)
- src/lexer.ts (8 hunks)
- src/operators.ts (2 hunks)
- src/parser.ts (6 hunks)
- src/translator.ts (2 hunks)
- src/types.ts (2 hunks)
- test/scenarios/comparisons/data.ts (1 hunks)
- test/scenarios/comparisons/template.jt (1 hunks)
- test/scenarios/paths/block.jt (1 hunks)
- test/scenarios/paths/data.ts (1 hunks)
- test/scenarios/paths/json_path.jt (1 hunks)
- test/scenarios/paths/simple_path.jt (1 hunks)
- test/test_engine.ts (1 hunks)
Files skipped from review due to trivial changes (3)
- test/scenarios/comparisons/data.ts
- test/scenarios/paths/simple_path.jt
- test/test_engine.ts
Additional Context Used
LanguageTool (24)
readme.md (24)
Near line 64: Possible subject-verb agreement error.
Context: ...es Template is a set of statements and result the last statement is the output of the...
Near line 74: Possible missing preposition found.
Context: ...a = 1; let b = a + 2; a + b; ``` Refer this [example](test/scenarios/assignments/te...
Near line 84: Possible missing preposition found.
Context: ...itionsjs a > b ? a : c;
Refer this [example](test/scenarios/conditions/tem...
Near line 119: Possible missing preposition found.
Context: ...nd.
refers to current context. Refer this [example](test/scenarios/selectors/cont...
Near line 121: Possible missing preposition found.
Context: ...ltName" property of the bindings. Refer this [example](test/scenarios/bindings/templ...
Near line 146: Possible missing preposition found.
Context: ... // { "c": 3, "some key": 4} ``` Refer this [example](test/scenarios/objects/templa...
Near line 174: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...tions are short to express the intention and it is convenient sometimes. #### Async...
Near line 185: Possible missing comma found.
Context: ...`` Note: When we want to use async functions then we need to create template engine ...
Near line 185: Consider adding a comma.
Context: ...eate`. If you create a template this way then it will be created as an async function...
Near line 191: Possible missing preposition found.
Context: ... = await doSomething(.a, .b) ``` Refer this [example](test/scenarios/functions/temp...
Near line 199: The verb ‘generate’ does not usually follow articles like ‘the’. Check that ‘generate’ is spelled correctly; using ‘generate’ as a noun may be non-standard.
Context: ...as direct property access statements in the generate javascript code.a.b.c
gets translate...
Near line 200: Did you mean “?”
Context: ...script code.a.b.c
gets translated toa?.b?.c
so they are very fast compared to ...
Near line 210: It seems that the correct verb form here is “get”.
Context: ...e details. #### Rich Paths Rich paths gets converted complex code to support diffe...
Near line 217: Possible missing preposition found.
Context: ...-{"a": [{ "b": [{"c": 2}]}]}
Refer this [example](test/scenarios/paths/rich_pat...
Near line 221: Consider replacing this word to strengthen your wording.
Context: .... Note: This is an experimental feature and may not support all the features of JSO...
Near line 232: Possible missing preposition found.
Context: ....b.c; let y = a."some key".c ``` Refer this [example](test/scenarios/selectors/temp...
Near line 251: Possible missing preposition found.
Context: ...a..c; let y = a.."some key"; ``` Refer this [example](test/scenarios/selectors/temp...
Near line 261: Possible missing preposition found.
Context: ...ray let z = a['some key'].c; ``` Refer this [example](test/scenarios/filters/array_...
Near line 270: Possible missing preposition found.
Context: ...some key1', 'some key2')].c; ``` Refer this [example](test/scenarios/filters/array_...
Near line 287: Possible missing preposition found.
Context: ...ll properties except a and b ``` Refer this [example](test/scenarios/filters/object...
Near line 307: Possible missing preposition found.
Context: ... let x = obj.([.a+1, .b+2]); ``` Refer this [example](test/scenarios/paths/block.jt...
Near line 347: Possible missing preposition found.
Context: ...ower compare to the simple paths. Refer this [example](test/scenarios/paths/options....
Near line 371: Two consecutive dots
Context: ... single line (//
) and block comments (/* .. */
). Refer this [example](test/scenari...
Near line 392: Consider using a more formal alternative.
Context: ... see you contribute to RudderStack. Get more information on how to contribute [here](CONTRIB...
Additional comments not posted (21)
test/scenarios/paths/json_path.jt (3)
2-2
: Valid JSON Path expression for selecting thefoo
property.
3-3
: Valid JSON Path expression for selecting all items from theitems
array.
4-4
: Valid JSON Path expression for selecting items where the propertya
is greater than 1.test/scenarios/paths/block.jt (1)
3-4
: Correct use of@
symbol to reference the current context in block expressions.test/scenarios/comparisons/template.jt (2)
21-23
: Correct implementation of thein
andnin
keywords for checking membership and non-membership.
24-33
: Correct implementation of thecontains
,size
,empty
, andsubsetof
functions for various data structures.src/operators.ts (2)
26-26
: UpdatedcontainsStrict
function to correctly handle both strings and arrays.
77-88
: New functions inbinaryOperators
are correctly implemented and enhance the template engine's capabilities.test/scenarios/paths/data.ts (1)
72-117
: New test scenario forjson_path.jt
correctly sets up data and expected outputs for testing JSON Path expressions.src/types.ts (2)
11-16
: New keywords added to theKeyword
enum are necessary for supporting JSON Path functionality and various utility operations.
100-100
: Addition of theJSON
path type to thePathType
enum supports the new JSON Path functionality.readme.md (1)
219-224
: Documentation update correctly introduces the new JSON Path feature as an experimental feature and provides a link to an example.src/lexer.ts (3)
84-86
: New methodmatchJsonPath
correctly matches the~j
token for JSON Path expressions.
89-89
: Updated methodmatchPathType
correctly includes matching for JSON Path expressions.
152-166
: New methodsmatchContains
,matchEmpty
,matchSize
,matchSubsetOf
correctly match the respective keywords for new operations.src/translator.ts (2)
690-696
: The implementation of theIN
andNOT_IN
operators intranslateINExpr
method is correctly handled. Good use of ternary operations to switch between inclusion and exclusion logic.
42-42
: The structure and organization of theJsonTemplateTranslator
class are well-maintained. The use of theKeyword
enum from thetypes
module aligns with the changes made for supporting JSON path syntax.src/parser.ts (4)
216-217
: Ensure correct handling of array filters.The updated method
parseArrayFilterExpr
now correctly distinguishes between array and object filter expressions based on the presence of a '?' character, which aligns with the PR's objective to enhance JSON path support.
301-303
: Validate JSON path support.The addition of JSON path type handling in
parsePathType
method correctly implements the new~j
path type as described in the PR summary. This is a crucial part of supporting JSON path syntax in the template engine.
312-312
: Check integration ofparsePathRoot
withinparsePath
.The integration of
parsePathRoot
withinparsePath
method is correctly done. It ensures that the root of the path is parsed based on the path type, which is essential for the correct functioning of path expressions in the template engine.
557-579
: Review changes toparseArrayFilterExpr
.The modifications to
parseArrayFilterExpr
to handle different types of filters (array, object) based on the presence of specific tokens (?
,*
) are in line with the PR's objectives to enhance JSON path support. This change improves the flexibility and accuracy of filter parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/parser.ts (6 hunks)
- src/translator.ts (3 hunks)
- src/types.ts (4 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/parser.ts
- src/translator.ts
- src/types.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- test/scenarios/paths/data.ts (1 hunks)
- test/scenarios/paths/json_path.jt (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- test/scenarios/paths/data.ts
- test/scenarios/paths/json_path.jt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/parser.ts (7 hunks)
- test/scenarios/paths/data.ts (1 hunks)
- test/scenarios/paths/json_path.jt (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- test/scenarios/paths/data.ts
- test/scenarios/paths/json_path.jt
Additional comments not posted (3)
src/parser.ts (3)
311-313
: Ensure that JSON path type is correctly recognized and handled.This script checks for the usage of
PathType.JSON
across the codebase to ensure it is handled correctly in all relevant scenarios.
570-590
: Review the logic for determiningexprType
inparseArrayFilterExpr
.This script checks how
parseArrayFilterExpr
is used in test cases, particularly focusing on the logic that determinesexprType
based on the lexer's state.
219-220
: Ensure correct handling of array filters.This script checks how
parseArrayFilterExpr
is used in test scenarios to ensure it handles different cases correctly.Verification successful
The test files
./test/scenarios/filters/data.ts
and./test/scenarios/bad_templates/data.ts
contain scenarios that involve array filters and potential errors related to array expressions. Additionally,./test/scenario.test.ts
includes a framework for running these scenarios.
./test/scenarios/filters/data.ts
includes a scenario withtemplatePath: 'array_filters.jt'
../test/scenarios/bad_templates/data.ts
includes a scenario withtemplatePath: 'bad_array_coalese_expr.jt'
.These scenarios suggest that array filters are indeed being tested, either directly or indirectly, through these templates.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct handling of array filters in various scenarios. # Test: Search for usage of `parseArrayFilterExpr` in test scenarios. rg --type typescript $'parseArrayFilterExpr' ./testLength of output: 85
Script:
#!/bin/bash # Description: Verify the correct handling of array filters in various scenarios. # Test: Search for usage of `parseArrayFilterExpr` in test scenarios. rg --type ts 'parseArrayFilterExpr' ./testLength of output: 42
Script:
#!/bin/bash # Description: Verify the correct handling of array filters by checking the definition and usage of `parseArrayFilterExpr`. # Search for the definition of `parseArrayFilterExpr`. rg 'parseArrayFilterExpr' --type ts # Search for the usage of `parseArrayFilterExpr` throughout the codebase. rg 'parseArrayFilterExpr' --type tsLength of output: 377
Script:
#!/bin/bash # Description: Verify the correct handling of array filters by checking for test cases involving array filters. # Search for test cases involving array filters. rg 'array filter' --type ts ./testLength of output: 36
Script:
#!/bin/bash # Description: Verify the correct handling of array filters by checking for test cases involving the lexer matching '['. # Search for test cases involving the lexer matching '['. rg "lexer.match('\\[')" --type ts ./testLength of output: 47
Script:
#!/bin/bash # Description: Verify the correct handling of array filters by checking for test cases involving array expressions or filters. # Search for test cases involving array expressions or filters. rg 'array' --type ts ./testLength of output: 269
Script:
#!/bin/bash # Description: Verify the correct handling of array filters by inspecting test cases in specific files. # Display the contents of the relevant test files. cat ./test/scenarios/filters/data.ts cat ./test/scenarios/bad_templates/data.ts cat ./test/scenario.test.tsLength of output: 4935
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 16
Outside diff range and nitpick comments (5)
src/utils/common.ts (2)
Line range hint
3-36
: Consider convertingCommonUtils
from a class to a module with exported functions, as it only contains static methods.Convert the class to a module to simplify the structure and avoid the unnecessary class overhead.
Line range hint
27-27
: ConvertCreateAsyncFunction
to an arrow function for consistency and to avoid function hoisting issues.- return async function () {}.constructor(...args); + return async (...args) => {};test/utils/scenario.ts (2)
Line range hint
6-26
: Consider convertingScenarioUtils
from a class to a module with exported functions, as it only contains static methods.Convert the class to a module to simplify the structure and avoid the unnecessary class overhead.
Line range hint
18-18
: Specify a more precise type thanany
for theevaluateScenario
method's return type.Define a specific return type to enhance type safety and predictability of the function.
src/engine.ts (1)
Line range hint
20-20
: Clarify the use ofthis
in static methods to avoid confusion.Ensure that
this
is used correctly in static methods or consider refactoring to avoid its use where it might lead to misunderstandings.Also applies to: 30-30, 35-35, 54-54, 61-61, 75-75, 79-79, 81-81
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- .gitignore (1 hunks)
- .nvmrc (1 hunks)
- src/constants.ts (1 hunks)
- src/engine.ts (5 hunks)
- src/parser.ts (9 hunks)
- src/translator.ts (3 hunks)
- src/types.ts (5 hunks)
- src/utils/common.test.ts (1 hunks)
- src/utils/common.ts (1 hunks)
- src/utils/converter.test.ts (1 hunks)
- src/utils/converter.ts (1 hunks)
- src/utils/index.ts (1 hunks)
- test/scenarios/mappings/data.ts (1 hunks)
- test/scenarios/mappings/mappings.json (1 hunks)
- test/types.ts (2 hunks)
- test/utils/scenario.ts (1 hunks)
Files skipped from review due to trivial changes (5)
- .gitignore
- .nvmrc
- src/constants.ts
- src/utils/index.ts
- test/scenarios/mappings/mappings.json
Additional Context Used
Biome (58)
src/engine.ts (17)
20-20: Using this in a static context can be confusing.
30-30: Using this in a static context can be confusing.
35-35: Using this in a static context can be confusing.
35-35: Using this in a static context can be confusing.
54-54: Using this in a static context can be confusing.
61-61: Using this in a static context can be confusing.
75-75: Using this in a static context can be confusing.
79-79: Using this in a static context can be confusing.
81-81: Using this in a static context can be confusing.
84-84: Unexpected any. Specify a different type.
84-84: Unexpected any. Specify a different type.
84-84: Unexpected any. Specify a different type.
4-5: All these imports are only used as types.
9-9: Don't use 'Function' as a type.
11-11: Don't use 'Function' as a type.
18-18: Don't use 'Function' as a type.
26-26: Don't use 'Function' as a type.
src/parser.ts (14)
159-159: Unexpected any. Specify a different type.
228-228: The assignment should not be in an expression.
509-509: Unexpected any. Specify a different type.
980-980: This type annotation is trivially inferred from its initialization.
1358-1358: Using this in a static context can be confusing.
1380-1380: Using this in a static context can be confusing.
1386-1386: Using this in a static context can be confusing.
1393-1393: Using this in a static context can be confusing.
1423-1423: Using this in a static context can be confusing.
3-40: Some named imports are only used as types.
533-533: Do not add then to an object.
1181-1181: Use Number.parseFloat instead of the equivalent global.
1187-1187: Use Number.parseInt instead of the equivalent global.
1196-1196: Use Number.parseFloat instead of the equivalent global.
src/translator.ts (7)
83-83: Unexpected any. Specify a different type.
224-224: Do not use template literals if interpolation and special-character handling are not needed.
704-704: Unexpected any. Specify a different type.
768-768: Do not use template literals if interpolation and special-character handling are not needed.
846-846: Using this in a static context can be confusing.
877-877: This type annotation is trivially inferred from its initialization.
10-43: Some named imports are only used as types.
src/types.ts (3)
105-105: Unexpected any. Specify a different type.
111-111: Unexpected any. Specify a different type.
124-124: Unexpected any. Specify a different type.
src/utils/common.ts (5)
3-36: Avoid classes that contain only static members.
4-4: Unexpected any. Specify a different type.
4-4: Unexpected any. Specify a different type.
27-27: This function expression can be turned into an arrow function.
1-1: Some named imports are only used as types.
src/utils/converter.ts (2)
19-102: Avoid classes that contain only static members.
1-9: Some named imports are only used as types.
test/scenarios/mappings/data.ts (1)
1-1: All these imports are only used as types.
test/types.ts (4)
5-5: Unexpected any. Specify a different type.
9-9: Unexpected any. Specify a different type.
10-10: Unexpected any. Specify a different type.
1-1: Some named imports are only used as types.
test/utils/scenario.ts (5)
1-1: A Node.js builtin module should be imported with the node: protocol.
2-2: A Node.js builtin module should be imported with the node: protocol.
6-26: Avoid classes that contain only static members.
18-18: Unexpected any. Specify a different type.
2-3: Some named imports are only used as types.
Additional comments not posted (6)
src/utils/common.test.ts (1)
5-66
: Ensure comprehensive test coverage forCommonUtils
.The test cases appear to cover various scenarios for each method in
CommonUtils
, ensuring that the utility functions behave as expected across different inputs.src/types.ts (2)
11-16
: Confirm the addition of new keywords.Ensure that the newly added keywords (
NOT_IN
,CONTAINS
,SUBSETOF
,EMPTY
,SIZE
) are properly integrated and utilized in the lexer and parser as intended.
Line range hint
77-101
: Validate the integration of newSyntaxType
andPathType
additions.Check that the new syntax types (
ALL_FILTER_EXPR
,OBJECT_INDEX_FILTER_EXPR
,RANGE_FILTER_EXPR
,OBJECT_FILTER_EXPR
,ARRAY_FILTER_EXPR
) and the new path type (JSON
) are correctly handled in the parser and translator modules.src/translator.ts (1)
724-724
: Ensure consistent handling of different filter types intranslateArrayFilterExpr
.The method
translateArrayFilterExpr
handles different types of filters, but it's crucial to ensure that all potential filter types are handled correctly. Please verify that all filter types that should be supported are indeed being handled here. If some are missing, consider adding the necessary logic.src/parser.ts (2)
48-48
: InitializepathTypesStack
directly in its declaration.This change simplifies the initialization of
pathTypesStack
by directly assigning an empty array, which is a cleaner approach.
309-311
: Ensure correct handling of JSON path type.The addition of JSON path type handling in
parsePathType
method aligns with the PR's objective to support JSON path syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Outside diff range and nitpick comments (9)
src/engine.ts (1)
Line range hint
20-20
: Avoid usingthis
in static methods as it can lead to confusion and potential errors.- return Function(DATA_PARAM_KEY, BINDINGS_PARAM_KEY, this.translate(templateOrExpr, options)); + return Function(DATA_PARAM_KEY, BINDINGS_PARAM_KEY, JsonTemplateEngine.translate(templateOrExpr, options));Also applies to: 30-30, 35-35, 54-54, 61-61, 75-75, 79-79, 81-81
src/translator.ts (1)
Line range hint
224-224
: Avoid using template literals where simple string concatenation would suffice.- code.push(`${dest} = ${result}.flat();`); + code.push(dest + " = " + result + ".flat();");Also applies to: 768-768
src/parser.ts (7)
Line range hint
159-159
: Specify a more specific type instead ofany
.Using
any
can lead to potential type safety issues. Consider specifying a more appropriate type.- private options?: EngineOptions; + private options?: EngineOptions | null;
Line range hint
509-509
: Specify a more specific type instead ofany
.Using
any
can lead to potential type safety issues. Consider specifying a more appropriate type.- let filter: Expression | undefined; + let filter: Expression | null;
Line range hint
980-980
: Remove unnecessary type annotation.The type is inferred from the initialization, making the explicit type annotation redundant.
- private loopCount: number = 0; + private loopCount = 0;
Line range hint
1358-1358
: Avoid usingthis
in static methods.Using
this
in static methods can lead to confusion and errors because static methods do not operate on an instance of the class.- this.prependFunctionID(pathExpr.root, fnExpr.id); + JsonTemplateParser.prependFunctionID(pathExpr.root, fnExpr.id);Also applies to: 1380-1380, 1386-1386, 1393-1393, 1423-1423
Line range hint
3-40
: Remove unused imports.Some imports are only used as types and can be removed to clean up the code.
- import { JsonTemplateLexerError, JsonTemplateParserError } from './errors'; + import type { JsonTemplateLexerError, JsonTemplateParserError } from './errors';
Line range hint
533-533
: Avoid adding then to an object.The use of
then
in this context is incorrect and could lead to runtime errors or unexpected behavior.- object.then = this.parseThenExpr(); + object.thenExpr = this.parseThenExpr();
Line range hint
1181-1181
: UseNumber.parseFloat
andNumber.parseInt
.Using
Number.parseFloat
andNumber.parseInt
instead of the globalparseFloat
andparseInt
functions is recommended to avoid potential issues in environments where these globals might be shadowed or redefined.- parseFloat(val); + Number.parseFloat(val); - parseInt(val, 10); + Number.parseInt(val, 10); - parseFloat(val); + Number.parseFloat(val);Also applies to: 1187-1187, 1196-1196
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- src/engine.ts (4 hunks)
- src/parser.ts (14 hunks)
- src/translator.ts (8 hunks)
- src/utils/common.test.ts (1 hunks)
- src/utils/common.ts (1 hunks)
- src/utils/converter.test.ts (1 hunks)
- src/utils/converter.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/utils/common.test.ts
- src/utils/converter.test.ts
Additional Context Used
Biome (39)
src/engine.ts (14)
20-20: Using this in a static context can be confusing.
30-30: Using this in a static context can be confusing.
35-35: Using this in a static context can be confusing.
35-35: Using this in a static context can be confusing.
54-54: Using this in a static context can be confusing.
61-61: Using this in a static context can be confusing.
75-75: Using this in a static context can be confusing.
79-79: Using this in a static context can be confusing.
81-81: Using this in a static context can be confusing.
4-5: All these imports are only used as types.
9-9: Don't use 'Function' as a type.
11-11: Don't use 'Function' as a type.
18-18: Don't use 'Function' as a type.
26-26: Don't use 'Function' as a type.
src/parser.ts (14)
159-159: Unexpected any. Specify a different type.
228-228: The assignment should not be in an expression.
509-509: Unexpected any. Specify a different type.
980-980: This type annotation is trivially inferred from its initialization.
1358-1358: Using this in a static context can be confusing.
1380-1380: Using this in a static context can be confusing.
1386-1386: Using this in a static context can be confusing.
1393-1393: Using this in a static context can be confusing.
1423-1423: Using this in a static context can be confusing.
3-40: Some named imports are only used as types.
533-533: Do not add then to an object.
1181-1181: Use Number.parseFloat instead of the equivalent global.
1187-1187: Use Number.parseInt instead of the equivalent global.
1196-1196: Use Number.parseFloat instead of the equivalent global.
src/translator.ts (7)
83-83: Unexpected any. Specify a different type.
224-224: Do not use template literals if interpolation and special-character handling are not needed.
704-704: Unexpected any. Specify a different type.
768-768: Do not use template literals if interpolation and special-character handling are not needed.
846-846: Using this in a static context can be confusing.
877-877: This type annotation is trivially inferred from its initialization.
10-43: Some named imports are only used as types.
src/utils/common.ts (3)
3-3: Unexpected any. Specify a different type.
3-3: Unexpected any. Specify a different type.
26-26: This function expression can be turned into an arrow function.
src/utils/converter.ts (1)
1-9: Some named imports are only used as types.
Additional comments not posted (3)
src/parser.ts (3)
48-48
: InitializepathTypesStack
directly in its declaration.This initialization ensures that
pathTypesStack
is never undefined, which simplifies the handling of this array throughout the class.
269-285
: RefactorparsePathRoot
to simplify token handling.This change uses a switch statement to handle different cases, which can be easier to read and maintain.
573-593
: RefactorparseArrayFilterExpr
to handle object filter expressions.This change introduces handling for object filter expressions within the
parseArrayFilterExpr
method, which is a significant enhancement for parsing capabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Outside diff range and nitpick comments (9)
src/translator.ts (6)
Line range hint
10-43
: Optimize imports by using explicit type imports where applicable.// Change these imports to type imports to prevent runtime inclusion where not necessary import type { ArrayExpression, AssignmentExpression, BinaryExpression, Expression, FunctionCallExpression, FunctionExpression, LiteralExpression, ObjectExpression, PathExpression, RangeFilterExpression, SelectorExpression, StatementsExpression, SyntaxType, UnaryExpression, TokenType, IndexFilterExpression, DefinitionExpression, SpreadExpression, LambdaArgExpression, ConditionalExpression, ObjectFilterExpression, ArrayFilterExpression, BlockExpression, PathOptions, PathType, ReturnExpression, ThrowExpression, LoopExpression, IncrementExpression, LoopControlExpression, Keyword, } from './types';
Line range hint
83-83
: Specify a more appropriate type instead ofany
.// Replace 'any' with a more specific type to improve type safety and code clarity private releaseVars(...args: string[]): void { let i = args.length; while (i--) { this.unusedVars.push(args[i]); } }
Line range hint
224-224
: Avoid using template literals when not necessary.// Simplify the string concatenation as interpolation is not needed here code.push('let ' + dest + ';');
Line range hint
771-771
: Avoid using template literals when not necessary.// Simplify the string concatenation as interpolation is not needed here code.push(varName + ' = Array.isArray(' + varName + ') ? ' + varName + ' : [' + varName + '];');
Line range hint
849-849
: Clarify the use ofthis
in a static context.// Refactor the method to either be non-static or avoid using `this` in a static context private static isToArray(expr: PathExpression, partNum: number): boolean { return this.getPathOptions(expr, partNum).toArray === true; }
Line range hint
880-880
: Remove unnecessary type annotation.// Since the type is inferred, the explicit annotation can be removed private static generateAssignmentCode(key: string, val: string, op: string = '='): string { return `${key}${op}${val};`; }src/parser.ts (3)
Line range hint
159-159
: Unexpected use ofany
type. Consider specifying a more explicit type to improve type safety.- catch (error: any) { + catch (error: Error) {Also applies to: 509-509
Line range hint
980-980
: The type annotation is trivially inferred from its initialization and can be omitted.- const exprVal: Expression = JsonTemplateEngine.createAsSync(expr).evaluate( + const exprVal = JsonTemplateEngine.createAsSync(expr).evaluate(
Line range hint
1358-1358
: Usingthis
in a static context can be confusing and potentially lead to errors if the static method is mistakenly thought to operate on an instance.- this.prependFunctionID(pathExpr.root, fnExpr.id); + JsonTemplateParser.prependFunctionID(pathExpr.root, fnExpr.id);Also applies to: 1380-1380, 1386-1386, 1393-1393, 1423-1423
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- src/lexer.ts (12 hunks)
- src/operators.ts (2 hunks)
- src/parser.ts (16 hunks)
- src/translator.ts (8 hunks)
- src/types.ts (6 hunks)
- test/scenarios/bad_templates/bad_regex.jt (1 hunks)
- test/scenarios/bad_templates/data.ts (1 hunks)
- test/scenarios/comparisons/data.ts (1 hunks)
- test/scenarios/comparisons/template.jt (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/operators.ts
- test/scenarios/comparisons/template.jt
Additional Context Used
Biome (33)
src/lexer.ts (7)
126-126: This type annotation is trivially inferred from its initialization.
131-131: This type annotation is trivially inferred from its initialization.
199-199: This type annotation is trivially inferred from its initialization.
294-294: Unexpected any. Specify a different type.
350-350: Using this in a static context can be confusing.
431-431: This variable implicitly has the any type.
2-3: Some named imports are only used as types.
src/parser.ts (14)
159-159: Unexpected any. Specify a different type.
228-228: The assignment should not be in an expression.
509-509: Unexpected any. Specify a different type.
980-980: This type annotation is trivially inferred from its initialization.
1358-1358: Using this in a static context can be confusing.
1380-1380: Using this in a static context can be confusing.
1386-1386: Using this in a static context can be confusing.
1393-1393: Using this in a static context can be confusing.
1423-1423: Using this in a static context can be confusing.
3-40: Some named imports are only used as types.
533-533: Do not add then to an object.
1181-1181: Use Number.parseFloat instead of the equivalent global.
1187-1187: Use Number.parseInt instead of the equivalent global.
1196-1196: Use Number.parseFloat instead of the equivalent global.
src/translator.ts (7)
83-83: Unexpected any. Specify a different type.
224-224: Do not use template literals if interpolation and special-character handling are not needed.
704-704: Unexpected any. Specify a different type.
771-771: Do not use template literals if interpolation and special-character handling are not needed.
849-849: Using this in a static context can be confusing.
880-880: This type annotation is trivially inferred from its initialization.
10-43: Some named imports are only used as types.
src/types.ts (3)
106-106: Unexpected any. Specify a different type.
112-112: Unexpected any. Specify a different type.
125-125: Unexpected any. Specify a different type.
test/scenarios/bad_templates/data.ts (1)
1-1: All these imports are only used as types.
test/scenarios/comparisons/data.ts (1)
1-1: All these imports are only used as types.
Additional comments not posted (8)
test/scenarios/comparisons/data.ts (1)
26-39
: The additions to the test data are consistent with the existing structure.test/scenarios/bad_templates/data.ts (1)
28-31
: The addition of the new scenario for testing invalid regular expressions is consistent with the existing structure.src/types.ts (1)
11-16
: The updates to enums and interfaces align well with the new features introduced in the PR.Also applies to: 38-38, 78-78, 102-102, 197-198, 204-204, 267-275
src/lexer.ts (1)
84-86
: The additions and updates to the methods inJsonTemplateLexer
are well-implemented and support the new JSON path syntax and other features effectively.Also applies to: 89-89, 93-93, 152-166, 169-169, 282-287, 322-323, 346-346, 647-655, 665-695
src/translator.ts (2)
42-42
: Use explicit type imports for clarity and maintainability.
690-696
: Refactor the IN and NOT IN logic for clarity.src/parser.ts (2)
48-48
: Initialization ofpathTypesStack
directly in its declaration is a good practice.
Line range hint
1181-1181
: UseNumber.parseFloat
andNumber.parseInt
instead of the equivalent global functions to avoid potential issues in non-browser environments.
[REFACTOR_SUGGESTion]- parseFloat(val), - parseInt(val, 10), - parseFloat(val), + Number.parseFloat(val), + Number.parseInt(val, 10), + Number.parseFloat(val),Also applies to: 1187-1187, 1196-1196
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
Outside diff range and nitpick comments (1)
src/translator.ts (1)
Line range hint
844-844
: Clarify the use ofthis
in a static context.- return this.getPathOptions(expr, partNum).toArray === true; + return JsonTemplateTranslator.getPathOptions(expr, partNum).toArray === true;
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- src/lexer.ts (12 hunks)
- src/operators.ts (2 hunks)
- src/parser.ts (16 hunks)
- src/translator.ts (7 hunks)
- src/types.ts (6 hunks)
- test/scenarios/comparisons/data.ts (1 hunks)
- test/scenarios/comparisons/template.jt (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- test/scenarios/comparisons/template.jt
Additional Context Used
Biome (32)
src/lexer.ts (7)
126-126: This type annotation is trivially inferred from its initialization.
131-131: This type annotation is trivially inferred from its initialization.
211-211: This type annotation is trivially inferred from its initialization.
306-306: Unexpected any. Specify a different type.
362-362: Using this in a static context can be confusing.
443-443: This variable implicitly has the any type.
2-3: Some named imports are only used as types.
src/parser.ts (14)
159-159: Unexpected any. Specify a different type.
228-228: The assignment should not be in an expression.
509-509: Unexpected any. Specify a different type.
1014-1014: This type annotation is trivially inferred from its initialization.
1392-1392: Using this in a static context can be confusing.
1414-1414: Using this in a static context can be confusing.
1420-1420: Using this in a static context can be confusing.
1427-1427: Using this in a static context can be confusing.
1457-1457: Using this in a static context can be confusing.
3-40: Some named imports are only used as types.
533-533: Do not add then to an object.
1215-1215: Use Number.parseFloat instead of the equivalent global.
1221-1221: Use Number.parseInt instead of the equivalent global.
1230-1230: Use Number.parseFloat instead of the equivalent global.
src/translator.ts (7)
83-83: Unexpected any. Specify a different type.
224-224: Do not use template literals if interpolation and special-character handling are not needed.
699-699: Unexpected any. Specify a different type.
766-766: Do not use template literals if interpolation and special-character handling are not needed.
844-844: Using this in a static context can be confusing.
875-875: This type annotation is trivially inferred from its initialization.
10-43: Some named imports are only used as types.
src/types.ts (3)
108-108: Unexpected any. Specify a different type.
114-114: Unexpected any. Specify a different type.
127-127: Unexpected any. Specify a different type.
test/scenarios/comparisons/data.ts (1)
1-1: All these imports are only used as types.
GitHub Check Runs (1)
Check for formatting & lint errors failure (3)
src/translator.ts: [failure] 42-42:
'Keyword' is defined but never used
Additional comments not posted (9)
test/scenarios/comparisons/data.ts (1)
26-41
: The additions to the test scenarios are consistent with the expected outputs.src/operators.ts (2)
26-26
: The update tocontainsStrict
to handle both strings and arrays enhances its functionality and aligns with the new JSON path features.
77-93
: The addition of new binary operators forsize
,empty
, andsubsetof
is correctly implemented and supports the new JSON path operations.src/types.ts (3)
11-18
: The addition of new keywords to theKeyword
enum supports the expanded functionality of the JSON path syntax.
104-104
: The addition of theJSON
path type to thePathType
enum is crucial for supporting JSON path queries.
199-206
: The new interfaces for filter expressions are well-defined and support the complex querying capabilities introduced.src/lexer.ts (2)
84-93
: The addition ofmatchJsonPath
and the update tomatchPathType
andmatchPath
correctly implement the lexer's ability to recognize the new JSON path type.
152-182
: The methods added to match the new keywords (contains
,empty
,size
,subsetof
,anyof
,noneof
,notIN
) are correctly implemented and enhance the lexer's capabilities.src/parser.ts (1)
48-48
: Refactor to initializepathTypesStack
directly in its declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Outside diff range and nitpick comments (8)
src/types.ts (1)
Line range hint
108-108
: Specify a more appropriate type instead ofany
to improve type safety and code clarity.// Example fix for one of the lines: export type Token = { type: TokenType; value: string | number | boolean | null | undefined; // Specify possible types instead of any range: [number, number]; };Also applies to: 114-114, 127-127
src/parser.ts (7)
Line range hint
159-159
: Specify a more specific type instead of usingany
.- catch (error: any) { + catch (error: Error) {
Line range hint
512-512
: Specify a more specific type instead of usingany
.- } catch (error: any) { + } catch (error: Error) {
Line range hint
1017-1017
: This type annotation is trivially inferred from its initialization.Consider removing the explicit type annotation to simplify the code.
Line range hint
1411-1417
: Usingthis
in a static context can be confusing.Consider refactoring to avoid using
this
in static methods or changing the method's static nature if it needs to access instance-specific data.Also applies to: 1424-1424, 1454-1454
Line range hint
3-40
: Some named imports are only used as types.Consider importing these only where needed to reduce the scope and improve clarity.
Line range hint
536-536
: Do not add then to an object.It seems like a typo or incorrect operation. Please clarify or correct the code.
Line range hint
1218-1218
: UseNumber.parseFloat
andNumber.parseInt
instead of the equivalent global functions.- parseFloat(val) + Number.parseFloat(val) - parseInt(val, 10) + Number.parseInt(val, 10)Also applies to: 1224-1224, 1233-1233
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/operators.ts (3 hunks)
- src/parser.ts (16 hunks)
- src/translator.ts (12 hunks)
- src/types.ts (7 hunks)
- test/scenarios/functions/array_functions.jt (1 hunks)
- test/scenarios/functions/data.ts (1 hunks)
- test/scenarios/standard_functions/data.ts (1 hunks)
- test/scenarios/standard_functions/template.jt (1 hunks)
Files skipped from review due to trivial changes (2)
- test/scenarios/functions/array_functions.jt
- test/scenarios/standard_functions/template.jt
Additional Context Used
Biome (26)
src/operators.ts (1)
171-171: Do not use template literals if interpolation and special-character handling are not needed.
src/parser.ts (13)
159-159: Unexpected any. Specify a different type.
228-228: The assignment should not be in an expression.
512-512: Unexpected any. Specify a different type.
1017-1017: This type annotation is trivially inferred from its initialization.
1411-1411: Using this in a static context can be confusing.
1417-1417: Using this in a static context can be confusing.
1424-1424: Using this in a static context can be confusing.
1454-1454: Using this in a static context can be confusing.
3-40: Some named imports are only used as types.
536-536: Do not add then to an object.
1218-1218: Use Number.parseFloat instead of the equivalent global.
1224-1224: Use Number.parseInt instead of the equivalent global.
1233-1233: Use Number.parseFloat instead of the equivalent global.
src/translator.ts (7)
84-84: Unexpected any. Specify a different type.
229-229: Do not use template literals if interpolation and special-character handling are not needed.
720-720: Unexpected any. Specify a different type.
787-787: Do not use template literals if interpolation and special-character handling are not needed.
865-865: Using this in a static context can be confusing.
896-896: This type annotation is trivially inferred from its initialization.
10-42: Some named imports are only used as types.
src/types.ts (3)
108-108: Unexpected any. Specify a different type.
114-114: Unexpected any. Specify a different type.
127-127: Unexpected any. Specify a different type.
test/scenarios/functions/data.ts (1)
1-1: All these imports are only used as types.
test/scenarios/standard_functions/data.ts (1)
1-1: All these imports are only used as types.
Additional comments not posted (9)
test/scenarios/standard_functions/data.ts (1)
1-1
: The import is correctly used as a type, which is a good practice in TypeScript to avoid runtime import overhead.test/scenarios/functions/data.ts (1)
1-1
: The import is correctly used as a type, which is a good practice in TypeScript to avoid runtime import overhead.src/operators.ts (2)
26-26
: The update tocontainsStrict
to include arrays in its checks aligns with the new JSON path functionalities.
77-79
: The addition of new operators for JSON path operations such as=~
,size
,empty
,subsetof
, andanyof
is well-implemented and enhances the flexibility of the engine.Also applies to: 86-93
src/types.ts (1)
11-18
: The additions to the enums and new interface definitions are well-aligned with the new JSON path functionalities and enhance the type safety of the engine.Also applies to: 40-40, 80-80, 104-104, 199-199, 206-206, 269-277
src/translator.ts (2)
10-10
: The addition of new imports and the handling of standard functions are well-implemented. The logic to include functions only if they are used is a good performance optimization.Also applies to: 43-43, 52-53, 95-98
720-720
: Address past comments regarding type safety and the use of switch statements for clarity.Also applies to: 722-722, 743-743
src/parser.ts (2)
48-48
: Initialization ofpathTypesStack
directly in its declaration is a good practice.
228-228
: Avoid assignments within expressions to enhance code readability and maintainability.- while ((newParts = toArray(this.parsePathPart()))) { + newParts = toArray(this.parsePathPart()); + while (newParts) {Likely invalid or redundant comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/utils/converter.ts (1 hunks)
Additional Context Used
Biome (1)
src/utils/converter.ts (1)
1-12: Some named imports are only used as types.
Additional comments not posted (6)
src/utils/converter.ts (6)
15-20
: FunctionCreateObjectExpression
correctly initializes anObjectExpression
. Well done!
22-36
: FunctionfindOrCreateObjectPropExpression
efficiently manages object properties. Good use of array methods and conditional logic.
38-55
: FunctionprocessArrayIndexFilter
correctly handles different scenarios based on the type of the current output property AST. Efficient handling of array expressions.
57-82
: FunctionprocessAllFilter
effectively manipulates path expressions based on the presence of object filters. Good use of array methods and conditional logic.
84-118
: FunctionprocessFlatMapping
effectively processes flat mappings and handles different types of selectors and filters. Good integration of helper functions.
122-130
: FunctionconvertToObjectMapping
correctly initializes and builds an object expression from flat mappings. Efficient use of iteration and helper functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (7)
src/parser.ts (7)
Line range hint
159-159
: Specify a more explicit type thanany
for error handling.- } catch (error: any) { + } catch (error: Error) {
Line range hint
507-507
: Specify a more explicit type thanany
for error handling.- } catch (error: any) { + } catch (error: Error) {
Line range hint
1023-1023
: Remove unnecessary type annotation.- let key: Expression | string; + let key;
Line range hint
1420-1420
: Avoid usingthis
in static methods as it can lead to confusion.- return this.isArrayFilterExpressionSimple(part as ArrayFilterExpression); + return JsonTemplateParser.isArrayFilterExpressionSimple(part as ArrayFilterExpression); - return this.isSimplePath(pathExpr); + return JsonTemplateParser.isSimplePath(pathExpr); - return this.isRichPath(pathExpr); + return JsonTemplateParser.isRichPath(pathExpr); - return this.isRichPath(newPathExpr); + return JsonTemplateParser.isRichPath(newPathExpr);Also applies to: 1426-1426, 1433-1433, 1463-1463
Line range hint
3-40
: Remove unused imports that are only used as types.- import { JsonTemplateLexerError, JsonTemplateParserError } from './errors'; + import type { JsonTemplateLexerError, JsonTemplateParserError } from './errors';
Line range hint
531-531
: Avoid adding then to an object.- const thenExpr = this.parseConditionalBodyExpr(); + const thenExpression = this.parseConditionalBodyExpr();
Line range hint
1227-1227
: UseNumber.parseFloat
andNumber.parseInt
instead of the equivalent global functions.- return parseFloat(val); + return Number.parseFloat(val); - return parseInt(val, 10); + return Number.parseInt(val, 10); - return parseFloat(val); + return Number.parseFloat(val);Also applies to: 1233-1233, 1242-1242
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (28)
- src/operators.ts (4 hunks)
- src/parser.ts (18 hunks)
- test/scenarios/bad_templates/object_with_invalid_key.jt (1 hunks)
- test/scenarios/comparisons/anyof.jt (1 hunks)
- test/scenarios/comparisons/contains.jt (1 hunks)
- test/scenarios/comparisons/data.ts (1 hunks)
- test/scenarios/comparisons/empty.jt (1 hunks)
- test/scenarios/comparisons/ends_with.jt (1 hunks)
- test/scenarios/comparisons/ends_with_ignore_case.jt (1 hunks)
- test/scenarios/comparisons/eq.jt (1 hunks)
- test/scenarios/comparisons/ge.jt (1 hunks)
- test/scenarios/comparisons/gte.jt (1 hunks)
- test/scenarios/comparisons/in.jt (1 hunks)
- test/scenarios/comparisons/le.jt (1 hunks)
- test/scenarios/comparisons/lte.jt (1 hunks)
- test/scenarios/comparisons/ne.jt (1 hunks)
- test/scenarios/comparisons/noneof.jt (1 hunks)
- test/scenarios/comparisons/not_in.jt (1 hunks)
- test/scenarios/comparisons/regex.jt (1 hunks)
- test/scenarios/comparisons/size.jt (1 hunks)
- test/scenarios/comparisons/starts_with.jt (1 hunks)
- test/scenarios/comparisons/starts_with_ignore_case.jt (1 hunks)
- test/scenarios/comparisons/string_contains_ignore_case.jt (1 hunks)
- test/scenarios/comparisons/string_eq.jt (1 hunks)
- test/scenarios/comparisons/string_eq_ingore_case.jt (1 hunks)
- test/scenarios/comparisons/string_ne.jt (1 hunks)
- test/scenarios/comparisons/string_ne_ingore_case.jt (1 hunks)
- test/scenarios/comparisons/subsetof.jt (1 hunks)
Files not reviewed due to errors (1)
- src/operators.ts (no review received)
Files skipped from review due to trivial changes (25)
- test/scenarios/bad_templates/object_with_invalid_key.jt
- test/scenarios/comparisons/anyof.jt
- test/scenarios/comparisons/contains.jt
- test/scenarios/comparisons/empty.jt
- test/scenarios/comparisons/ends_with.jt
- test/scenarios/comparisons/ends_with_ignore_case.jt
- test/scenarios/comparisons/eq.jt
- test/scenarios/comparisons/ge.jt
- test/scenarios/comparisons/gte.jt
- test/scenarios/comparisons/in.jt
- test/scenarios/comparisons/le.jt
- test/scenarios/comparisons/lte.jt
- test/scenarios/comparisons/ne.jt
- test/scenarios/comparisons/noneof.jt
- test/scenarios/comparisons/not_in.jt
- test/scenarios/comparisons/regex.jt
- test/scenarios/comparisons/size.jt
- test/scenarios/comparisons/starts_with.jt
- test/scenarios/comparisons/starts_with_ignore_case.jt
- test/scenarios/comparisons/string_contains_ignore_case.jt
- test/scenarios/comparisons/string_eq.jt
- test/scenarios/comparisons/string_eq_ingore_case.jt
- test/scenarios/comparisons/string_ne.jt
- test/scenarios/comparisons/string_ne_ingore_case.jt
- test/scenarios/comparisons/subsetof.jt
Additional Context Used
Biome (13)
src/parser.ts (12)
159-159: Unexpected any. Specify a different type.
507-507: Unexpected any. Specify a different type.
1023-1023: This type annotation is trivially inferred from its initialization.
1420-1420: Using this in a static context can be confusing.
1426-1426: Using this in a static context can be confusing.
1433-1433: Using this in a static context can be confusing.
1463-1463: Using this in a static context can be confusing.
3-40: Some named imports are only used as types.
531-531: Do not add then to an object.
1227-1227: Use Number.parseFloat instead of the equivalent global.
1233-1233: Use Number.parseInt instead of the equivalent global.
1242-1242: Use Number.parseFloat instead of the equivalent global.
test/scenarios/comparisons/data.ts (1)
1-1: All these imports are only used as types.
Additional comments not posted (24)
test/scenarios/comparisons/data.ts (24)
5-10
: The scenario foranyof.jt
is correctly structured with appropriate true and false conditions.
12-17
: The scenario forcontains.jt
is correctly structured with appropriate true and false conditions.
19-24
: The scenario forempty.jt
is correctly structured with appropriate true and false conditions.
26-31
: The scenario forstring_contains_ignore_case.jt
is correctly structured with appropriate true and false conditions.
33-38
: The scenario forends_with.jt
is correctly structured with appropriate true and false conditions.
40-45
: The scenario forends_with_ignore_case.jt
is correctly structured with appropriate true and false conditions.
47-52
: The scenario foreq.jt
is correctly structured with appropriate true and false conditions.
54-59
: The scenario forge.jt
is correctly structured with appropriate true and false conditions.
61-66
: The scenario forgte.jt
is correctly structured with appropriate true and false conditions.
68-73
: The scenario forin.jt
is correctly structured with appropriate true and false conditions.
75-80
: The scenario forle.jt
is correctly structured with appropriate true and false conditions.
82-87
: The scenario forlte.jt
is correctly structured with appropriate true and false conditions.
89-94
: The scenario forne.jt
is correctly structured with appropriate true and false conditions.
96-101
: The scenario fornoneof.jt
is correctly structured with appropriate true and false conditions.
103-108
: The scenario fornot_in.jt
is correctly structured with appropriate true and false conditions.
110-115
: The scenario forregex.jt
is correctly structured with appropriate true and false conditions.
117-122
: The scenario forsize.jt
is correctly structured with appropriate true and false conditions.
124-129
: The scenario forstarts_with.jt
is correctly structured with appropriate true and false conditions.
131-136
: The scenario forstarts_with_ignore_case.jt
is correctly structured with appropriate true and false conditions.
138-143
: The scenario forstring_eq.jt
is correctly structured with appropriate true and false conditions.
145-150
: The scenario forstring_ne.jt
is correctly structured with appropriate true and false conditions.
152-157
: The scenario forstring_eq_ingore_case.jt
is correctly structured with appropriate true and false conditions.
159-164
: The scenario forstring_ne_ingore_case.jt
is correctly structured with appropriate true and false conditions.
173-177
: The scenario forsubsetof.jt
is correctly structured with appropriate true and false conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
Outside diff range and nitpick comments (14)
src/engine.ts (1)
40-49
: TheparseMappingPaths
method is a crucial addition for handling flat mapping paths. Consider adding more detailed comments explaining each step for maintainability.src/utils/converter.ts (1)
58-84
: The functionprocessAllFilter
is complex and could benefit from further comments explaining the logic, especially around the manipulation ofmatchedInputParts
and the conditions checked.Consider adding more detailed comments to explain the logic within
processAllFilter
, especially howmatchedInputParts
are used and the significance of the conditions checked.src/lexer.ts (1)
Line range hint
294-707
: Theadvance
method and its helper methods (scanRegularExpressions
,scanPunctuator
, etc.) are well-implemented. They handle the complexity of different token types effectively. However, consider adding more comments to explain the regular expression handling logic.Consider adding more detailed comments in the
scanRegularExpressions
method to explain the logic, especially the handling of escape characters and modifiers.src/translator.ts (5)
Line range hint
10-42
: Consider using explicit import paths for type-only imports.// Use explicit type-only imports to avoid potential side effects and clarify the intent. import type { ArrayExpression, AssignmentExpression, BinaryExpression, Expression, FunctionCallExpression, FunctionExpression, LiteralExpression, ObjectExpression, PathExpression, RangeFilterExpression, SelectorExpression, StatementsExpression, SyntaxType, UnaryExpression, TokenType, IndexFilterExpression, DefinitionExpression, SpreadExpression, LambdaArgExpression, ConditionalExpression, ObjectFilterExpression, ArrayFilterExpression, BlockExpression, PathOptions, PathType, ReturnExpression, ThrowExpression, LoopExpression, IncrementExpression, LoopControlExpression, } from './types';
Line range hint
85-85
: Specify a more appropriate type instead ofany
.// Replace 'any' with a more specific type to improve type safety and code clarity private releaseVars(...args: string[]): void { let i = args.length; while (i--) { this.unusedVars.push(args[i]); } }
Line range hint
230-230
: Avoid using template literals where not necessary.// Replace unnecessary template literals with simple strings or direct expressions code.push('return;');Also applies to: 253-253, 779-779
Line range hint
857-857
: Clarify the use ofthis
in static methods.// Refactor to avoid using `this` in static methods, which can lead to confusion and errors private static isToArray(expr: PathExpression, partNum: number): boolean { const options = JsonTemplateTranslator.getPathOptions(expr, partNum); return options.toArray === true; }
Line range hint
888-888
: Remove unnecessary type annotation.// Simplify by removing the redundant type annotation const code: string[] = [];src/parser.ts (6)
Line range hint
536-536
: Specify a more explicit type thanany
for error handling inparseConditionalBodyExpr
.- } catch (error: any) { + } catch (error: Error) {
Line range hint
1052-1052
: The type annotation forkey
inparseObjectKeyExpr
is trivially inferred from its initialization and can be omitted.- let key: Expression | string; + let key;
Line range hint
1454-1454
: Avoid usingthis
in static methods as it can lead to confusion and potential errors.- this.isArrayFilterExpressionSimple(part as ArrayFilterExpression); + JsonTemplateParser.isArrayFilterExpressionSimple(part as ArrayFilterExpression); - this.isSimplePathPart(part); + JsonTemplateParser.isSimplePathPart(part); - this.isSimplePath(pathExpr); + JsonTemplateParser.isSimplePath(pathExpr); - this.isRichPath(newPathExpr); + JsonTemplateParser.isRichPath(newPathExpr);Also applies to: 1460-1460, 1467-1467, 1498-1498
Line range hint
3-40
: Some imports in the file are only used as types and can be imported usingimport type
to clarify their usage and potentially optimize bundling.- import { BINDINGS_PARAM_KEY, DATA_PARAM_KEY, EMPTY_EXPR } from './constants'; + import type { BINDINGS_PARAM_KEY, DATA_PARAM_KEY, EMPTY_EXPR } from './constants';
Line range hint
560-560
: Avoid adding then to an object as it can lead to unexpected behavior or errors.- object.then = something; + // Correct usage depending on context
Line range hint
1257-1257
: UseNumber.parseFloat
andNumber.parseInt
instead of the equivalent global functions for better clarity and consistency.- parseFloat(val); + Number.parseFloat(val); - parseInt(val, 10); + Number.parseInt(val, 10); - parseFloat(val); + Number.parseFloat(val);Also applies to: 1263-1263, 1272-1272
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- src/engine.ts (3 hunks)
- src/lexer.ts (12 hunks)
- src/parser.ts (17 hunks)
- src/reverse_translator.ts (1 hunks)
- src/translator.ts (15 hunks)
- src/types.ts (10 hunks)
- src/utils/common.ts (1 hunks)
- src/utils/converter.ts (1 hunks)
- src/utils/transalator.ts (1 hunks)
- test/scenarios/context_variables/filter.jt (1 hunks)
- test/scenarios/filters/object_filters.jt (1 hunks)
- test/scenarios/filters/object_indexes.jt (1 hunks)
- test/scenarios/objects/template.jt (2 hunks)
- test/scenarios/return/data.ts (1 hunks)
- test/scenarios/return/return_no_value.jt (1 hunks)
- test/utils/scenario.ts (1 hunks)
Files skipped from review due to trivial changes (4)
- test/scenarios/context_variables/filter.jt
- test/scenarios/filters/object_filters.jt
- test/scenarios/objects/template.jt
- test/scenarios/return/return_no_value.jt
Additional Context Used
Biome (50)
src/engine.ts (12)
21-21: Using this in a static context can be confusing.
31-31: Using this in a static context can be confusing.
55-55: Using this in a static context can be confusing.
62-62: Using this in a static context can be confusing.
67-67: Using this in a static context can be confusing.
80-80: Using this in a static context can be confusing.
82-82: Using this in a static context can be confusing.
5-6: All these imports are only used as types.
10-10: Don't use 'Function' as a type.
12-12: Don't use 'Function' as a type.
19-19: Don't use 'Function' as a type.
27-27: Don't use 'Function' as a type.
src/lexer.ts (7)
126-126: This type annotation is trivially inferred from its initialization.
131-131: This type annotation is trivially inferred from its initialization.
211-211: This type annotation is trivially inferred from its initialization.
306-306: Unexpected any. Specify a different type.
362-362: Using this in a static context can be confusing.
443-443: This variable implicitly has the any type.
2-3: Some named imports are only used as types.
src/parser.ts (12)
169-169: Unexpected any. Specify a different type.
536-536: Unexpected any. Specify a different type.
1052-1052: This type annotation is trivially inferred from its initialization.
1454-1454: Using this in a static context can be confusing.
1460-1460: Using this in a static context can be confusing.
1467-1467: Using this in a static context can be confusing.
1498-1498: Using this in a static context can be confusing.
3-40: Some named imports are only used as types.
560-560: Do not add then to an object.
1257-1257: Use Number.parseFloat instead of the equivalent global.
1263-1263: Use Number.parseInt instead of the equivalent global.
1272-1272: Use Number.parseFloat instead of the equivalent global.
src/reverse_translator.ts (1)
1-34: Some named imports are only used as types.
src/translator.ts (7)
85-85: Unexpected any. Specify a different type.
230-230: Do not use template literals if interpolation and special-character handling are not needed.
253-253: Do not use template literals if interpolation and special-character handling are not needed.
779-779: Do not use template literals if interpolation and special-character handling are not needed.
857-857: Using this in a static context can be confusing.
888-888: This type annotation is trivially inferred from its initialization.
10-42: Some named imports are only used as types.
src/types.ts (3)
108-108: Unexpected any. Specify a different type.
114-114: Unexpected any. Specify a different type.
127-127: Unexpected any. Specify a different type.
src/utils/common.ts (2)
33-33: This function expression can be turned into an arrow function.
1-1: Some named imports are only used as types.
src/utils/converter.ts (1)
1-13: Some named imports are only used as types.
src/utils/transalator.ts (1)
1-1: Some named imports are only used as types.
test/scenarios/return/data.ts (1)
1-1: All these imports are only used as types.
test/utils/scenario.ts (3)
6-30: Avoid classes that contain only static members.
22-22: Unexpected any. Specify a different type.
2-3: Some named imports are only used as types.
Additional comments not posted (17)
test/scenarios/filters/object_indexes.jt (1)
7-7
: The use of spread syntax with object indexing is innovative, but the syntax{!["d"]}
is unclear. Please clarify or document its intended functionality.src/utils/transalator.ts (1)
4-10
: The functiontranslateLiteral
is well-implemented with clear handling of string types. Ensure that all possibleTokenType
values are appropriately handled.test/scenarios/return/data.ts (1)
23-28
: Ensure that the scenarios are correctly capturing the intended behavior, especially with different inputs leading to the same output.Verification successful
The extracted scenarios show that different inputs (3 and 2) are indeed leading to the same output (1) for the template 'return_value.jt'. This confirms that the scenarios are capturing the intended behavior of different inputs leading to the same output.
templatePath: 'return_value.jt', input: 3, output: 1
templatePath: 'return_value.jt', input: 2, output: 1
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the scenarios for 'return_value.jt'. # Test: Check if different inputs intentionally lead to the same output. cat test/scenarios/return/data.ts | grep 'return_value.jt'Length of output: 132
Script:
#!/bin/bash # Description: Extract and display the full scenarios for 'return_value.jt' to verify inputs and outputs. awk '/templatePath: .return_value.jt./,/},/' test/scenarios/return/data.tsLength of output: 218
src/engine.ts (1)
25-28
: The methodcompileAsAsync
is correctly updated to handleFlatMappingPaths[]
. Ensure that theCreateAsyncFunction
is robust against various input types.src/utils/converter.ts (4)
16-21
: The functionCreateObjectExpression
is well-defined and follows best practices for creating a new object expression.
23-37
: The functionfindOrCreateObjectPropExpression
effectively ensures that a property exists in the object, creating it if necessary. This is a good use of the find and conditional creation pattern.
39-56
: The functionprocessArrayIndexFilter
correctly handles the case where the current property is not an array by initializing it as one. It also handles missing elements at specific indices well.
124-132
: The functionconvertToObjectMapping
serves as a good entry point for converting flat mappings to object expressions. It is concise and delegates complexity to other functions effectively.src/types.ts (5)
11-18
: The additions to theKeyword
enum are correctly implemented and align with the new features introduced in the lexer.
40-40
: The addition ofREGEXP
to theTokenType
enum is appropriate given the new regular expression handling in the lexer.
Line range hint
80-104
: The updates toSyntaxType
andPathType
enums are correctly implemented and necessary for the new JSON path functionality.
130-132
: ThePathOptionsExpression
interface is a good addition for type safety and clarity in handling path options within expressions.
204-205
: TheAllFilterExpression
interface is correctly defined as an empty interface, which is appropriate for its usage in filtering expressions.src/reverse_translator.ts (2)
39-110
: TheJsonTemplateReverseTranslator
class is well-structured and thetranslate
method effectively handles different expression types using a switch statement.
112-462
: The translation methods are well-implemented, each handling a specific type of expression. The use of helper methods liketranslateWithWrapper
enhances readability and maintainability.src/lexer.ts (2)
Line range hint
84-182
: The methods added to theJsonTemplateLexer
class for matching various new keywords and path types are correctly implemented and follow the established pattern in the lexer.
152-182
: The methods for matching new keywords related to the JSON path functionality (matchContains
,matchEmpty
, etc.) are correctly implemented. These methods enhance the lexer's capability to recognize more complex expressions.
@@ -137,10 +148,10 @@ export class JsonTemplateParser { | |||
if (!path.root || typeof path.root === 'object' || path.root === DATA_PARAM_KEY) { | |||
throw new JsonTemplateParserError('Invalid assignment path'); | |||
} | |||
if (JsonTemplateParser.isRichPath(expr as PathExpression)) { | |||
if (!JsonTemplateParser.isSimplePath(expr as PathExpression)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify a more explicit type than any
for error handling in parseBaseExpr
.
- } catch (error: any) {
+ } catch (error: Error) {
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
if (!JsonTemplateParser.isSimplePath(expr as PathExpression)) { | |
if (!JsonTemplateParser.isSimplePath(expr as PathExpression)) { | |
// existing code | |
} | |
// existing code | |
} catch (error: Error) { | |
// existing error handling code | |
} |
src/utils/transalator.ts
Outdated
@@ -0,0 +1,10 @@ | |||
import { TokenType, Literal } from '../types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using type-only imports for TokenType
and Literal
as they are only used as types here.
- import { TokenType, Literal } from '../types';
+ import type { TokenType, Literal } from '../types';
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
import { TokenType, Literal } from '../types'; | |
import type { TokenType, Literal } from '../types'; |
src/utils/converter.ts
Outdated
function processFlatMapping(flatMapping: FlatMappingAST, outputAST: ObjectExpression) { | ||
let currentOutputPropsAST = outputAST.props; | ||
const currentInputAST = flatMapping.input; | ||
|
||
const numOutputParts = flatMapping.output.parts.length; | ||
for (let i = 0; i < numOutputParts; i++) { | ||
const outputPart = flatMapping.output.parts[i]; | ||
|
||
if (outputPart.type === SyntaxType.SELECTOR && outputPart.prop?.value) { | ||
const key = outputPart.prop.value; | ||
|
||
if (i === numOutputParts - 1) { | ||
currentOutputPropsAST.push({ | ||
type: SyntaxType.OBJECT_PROP_EXPR, | ||
key, | ||
value: currentInputAST, | ||
} as ObjectPropExpression); | ||
break; | ||
} | ||
|
||
const currentOutputPropAST = findOrCreateObjectPropExpression(currentOutputPropsAST, key); | ||
let objectExpr: ObjectExpression = currentOutputPropAST.value as ObjectExpression; | ||
const nextOutputPart = flatMapping.output.parts[i + 1] as ArrayFilterExpression; | ||
if (nextOutputPart.filter?.type === SyntaxType.ALL_FILTER_EXPR) { | ||
objectExpr = processAllFilter(currentInputAST, currentOutputPropAST); | ||
} else if (nextOutputPart.filter?.type === SyntaxType.ARRAY_INDEX_FILTER_EXPR) { | ||
objectExpr = processArrayIndexFilter( | ||
currentOutputPropAST, | ||
nextOutputPart.filter as IndexFilterExpression, | ||
); | ||
} | ||
currentOutputPropsAST = objectExpr.props; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function processFlatMapping
is handling multiple types of filters and adjusting the output properties accordingly. However, the handling of numOutputParts
and the loop could be clarified with additional comments or refactoring to improve readability.
Consider refactoring processFlatMapping
to separate concerns more clearly, possibly by extracting parts of the logic into smaller, more focused functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/reverse_translator.ts (1 hunks)
Additional context used
Biome
src/reverse_translator.ts
[error] 1-34: Some named imports are only used as types.
Additional comments not posted (40)
src/reverse_translator.ts (40)
1-34
: Use type-only imports for types that are not used as values to clarify their usage and optimize bundling.
39-44
: Initialization ofoptions
in the constructor is straightforward and correct.
46-50
: The methodtranslate
correctly formats the code string by replacing unnecessary spaces between dots. This is a good practice for code cleanliness.
52-115
: ThetranslateExpression
method handles a wide range of expression types, ensuring that each type is directed to its specific translation method. This is a good example of using a switch-case for type discrimination.
118-120
: The methodtranslateArrayFilterExpression
correctly delegates the translation of the filter expression. This maintains modularity in the code.
122-134
: The methodtranslateRangeFilterExpression
constructs a range filter expression in a clear and concise manner. The use of an array to build the string is efficient for this purpose.
136-138
: The methodtranslateArrayIndexFilterExpression
is concise and delegates the translation to another method, maintaining simplicity and reusability.
140-149
: The methodtranslateObjectIndexFilterExpression
effectively constructs the object index filter expression, handling theexclude
flag correctly.
151-162
: The methodtranslateSelectorExpression
handles string properties correctly by escaping them when necessary. This is crucial for preventing injection attacks or errors in the output.
164-166
: The methodtranslateWithWrapper
is a utility function that correctly wraps an expression with specified prefixes and suffixes, demonstrating good use of template literals.
168-176
: The methodtranslateObjectFilterExpression
uses conditional logic to determine the correct wrapper based on the filter type and path options. This is a good practice for handling multiple conditions.
178-180
: The methodtranslateLambdaArgExpression
is straightforward and correctly formats the lambda argument expression.
182-184
: The methodtranslateLoopControlExpression
is simple and correctly returns the control expression.
186-206
: The methodtranslateLoopExpression
constructs the loop expression comprehensively, handling initialization, test, and update expressions correctly.
208-210
: The methodtranslateReturnExpression
correctly formats the return expression, ensuring that it defaults to an empty expression if none is provided.
212-214
: The methodtranslateThrowExpression
is implemented correctly, ensuring that the thrown value is translated properly.
216-218
: The methodtranslateExpressions
efficiently handles multiple expressions, joining them with a specified separator. This is a good use of themap
function for transformation.
220-222
: The methodtranslateLambdaFunctionExpression
is concise and correctly delegates the body translation, maintaining clarity and simplicity.
224-236
: The methodtranslateRegularFunctionExpression
constructs the function expression clearly, handling parameters and the function body effectively.
238-252
: The methodtranslateFunctionExpression
handles both regular and lambda functions, demonstrating flexibility in function translation based on the function type.
254-275
: The methodtranslateFunctionCallExpression
effectively constructs the function call expression, handling different scenarios for object and parent presence.
277-283
: The methodtranslateAssignmentExpression
correctly constructs the assignment expression, ensuring that the path, operator, and value are properly formatted.
285-298
: The methodtranslateDefinitionExpression
handles variable definitions effectively, particularly managing the scenario where variables are derived from an object.
300-317
: The methodtranslateConditionalExpression
is implemented well, handling the conditional logic and formatting the expression correctly.
319-330
: The methodtranslatePathType
uses a switch-case to handle different path types, returning the appropriate string representation for each.
332-340
: The methodtranslatePathRootString
handles special cases forBINDINGS_PARAM_KEY
andDATA_PARAM_KEY
correctly, ensuring that the appropriate symbols are returned based on the path type.
342-355
: The methodtranslatePathRoot
effectively handles both string and expression types for the root, demonstrating flexibility in handling path roots.
357-373
: The methodtranslatePathOptions
constructs the path options string correctly, handling optional properties likeitem
andindex
.
375-393
: The methodtranslatePathParts
constructs the path parts string effectively, handling different types of expressions and their options.
395-404
: The methodtranslatePathExpression
comprehensively constructs the path expression, integrating type, root, options, and parts translation effectively.
407-412
: The methodtranslateIncrementExpression
handles both postfix and prefix increment operations correctly, demonstrating flexibility in expression formatting.
414-416
: The methodtranslateUnaryExpression
is implemented correctly, ensuring that the unary operator and its argument are formatted properly.
418-424
: The methodtranslateBlockExpression
constructs the block expression effectively, handling multiple statements within a block.
426-428
: The methodtranslateSpreadExpression
correctly handles the spread operator, demonstrating good use of template literals for expression formatting.
430-435
: The methodtranslateObjectExpression
constructs the object expression clearly, handling properties and ensuring proper formatting with new lines.
438-451
: The methodtranslateObjectPropExpression
handles both string and expression types for keys, demonstrating flexibility and correctness in property translation.
454-460
: The methodtranslateArrayExpression
constructs the array expression effectively, handling elements and ensuring proper formatting with commas.
462-464
: The methodtranslateLiteralExpression
correctly delegates to thetranslateLiteral
utility function, ensuring that literals are translated based on their type and value.
466-468
: The methodtranslateStatementsExpression
handles multiple statements effectively, joining them with a newline and semicolon for clarity.
470-474
: The methodtranslateBinaryExpression
constructs the binary expression correctly, ensuring that the operands and operator are formatted properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/engine.ts (3 hunks)
- src/reverse_translator.ts (1 hunks)
- src/types.ts (10 hunks)
- src/utils/converter.ts (1 hunks)
- test/scenarios/mappings/data.ts (1 hunks)
- test/scenarios/mappings/invalid_mappings.json (1 hunks)
Files skipped from review due to trivial changes (1)
- test/scenarios/mappings/invalid_mappings.json
Files skipped from review as they are similar to previous changes (1)
- test/scenarios/mappings/data.ts
Additional context used
Biome
src/engine.ts
[error] 21-21: Using this in a static context can be confusing.
[error] 31-31: Using this in a static context can be confusing.
[error] 56-56: Using this in a static context can be confusing.
[error] 63-63: Using this in a static context can be confusing.
[error] 68-68: Using this in a static context can be confusing.
[error] 81-81: Using this in a static context can be confusing.
[error] 83-83: Using this in a static context can be confusing.
[error] 5-6: All these imports are only used as types.
[error] 10-10: Don't use 'Function' as a type.
[error] 12-12: Don't use 'Function' as a type.
[error] 19-19: Don't use 'Function' as a type.
[error] 27-27: Don't use 'Function' as a type.
src/utils/converter.ts
[error] 1-13: Some named imports are only used as types.
src/types.ts
[error] 108-108: Unexpected any. Specify a different type.
[error] 114-114: Unexpected any. Specify a different type.
[error] 127-127: Unexpected any. Specify a different type.
src/reverse_translator.ts
[error] 1-34: Some named imports are only used as types.
Additional comments not posted (3)
src/engine.ts (1)
25-28
: Refactor to avoid usingthis
in static methods, which can lead to confusion and potential errors.- return CreateAsyncFunction(DATA_PARAM_KEY, BINDINGS_PARAM_KEY, this.translate(templateOrExpr, options)); + return CreateAsyncFunction(DATA_PARAM_KEY, BINDINGS_PARAM_KEY, JsonTemplateEngine.translate(templateOrExpr, options));Likely invalid or redundant comment.
src/types.ts (2)
40-40
: Validate the new token and syntax types to ensure they are properly used in lexer and parser implementations.Also applies to: 80-80, 103-104
Verification successful
The new token and syntax types (REGEXP, ALL_FILTER_EXPR, JSON, UNKNOWN) are properly integrated and used in the lexer and parser implementations. Here are the key points:
Lexer Implementation:
- The
REGEXP
token type is used in the lexer to identify and handle regular expressions.- The
UNKNOWN
token type is used to handle unknown tokens and throw appropriate errors.Parser Implementation:
- The
ALL_FILTER_EXPR
syntax type is used in the parser to handle specific filter expressions.- The
JSON
path type is used to handle JSON path types in the parser.The code snippets confirm that these types are correctly implemented and used in the relevant contexts.
src/lexer.ts
:
- REGEXP: Lines around
token.type === TokenType.REGEXP
- UNKNOWN: Lines around
JsonTemplateLexer.throwError(MESSAGES.UNKNOWN_TOKEN)
src/parser.ts
:
- ALL_FILTER_EXPR: Lines around
type: SyntaxType.ALL_FILTER_EXPR
- JSON: Lines around
return this.createPathResult(PathType.JSON)
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for proper usage of new token and syntax types. # Test: Search for the usage of new types. Expect: Proper implementation in lexer and parser. rg --type ts 'REGEXP|ALL_FILTER_EXPR|JSON|UNKNOWN'Length of output: 2215
Script:
#!/bin/bash # Extract relevant code snippets from lexer and parser implementations for detailed examination. # Extract lines around the usage of REGEXP and UNKNOWN in lexer rg --type ts -A 5 -B 5 'REGEXP|UNKNOWN' src/lexer.ts # Extract lines around the usage of ALL_FILTER_EXPR and JSON in parser rg --type ts -A 5 -B 5 'ALL_FILTER_EXPR|JSON' src/parser.tsLength of output: 3323
11-18
: Ensure the newly added keywords in theKeyword
enum are correctly implemented throughout the codebase.Verification successful
The new keywords in the
Keyword
enum are correctly implemented throughout the codebase. They are used in the following files:
src/lexer.ts
: For matching keyword values.src/parser.ts
: For parsing operations.src/reverse_translator.ts
: For translating expressions.src/utils/common.test.ts
: For testing purposes.These usages indicate that the new keywords are integrated into the codebase and are being tested.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new keywords in the codebase. # Test: Search for the new keywords. Expect: Proper implementation in relevant files. rg --type ts 'NOT_IN|CONTAINS|SUBSETOF|ANYOF|NONEOF|EMPTY|SIZE'Length of output: 1678
src/engine.ts
Outdated
import { EngineOptions, Expression, FlatMappingPaths } from './types'; | ||
import { CreateAsyncFunction, convertToObjectMapping } from './utils'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use type-only imports for entities used solely as types to clarify their usage and optimize bundling.
- import { EngineOptions, Expression, FlatMappingPaths } from './types';
+ import type { EngineOptions, Expression, FlatMappingPaths } from './types';
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
import { EngineOptions, Expression, FlatMappingPaths } from './types'; | |
import { CreateAsyncFunction, convertToObjectMapping } from './utils'; | |
import type { EngineOptions, Expression, FlatMappingPaths } from './types'; | |
import { CreateAsyncFunction, convertToObjectMapping } from './utils'; |
src/utils/converter.ts
Outdated
function processFlatMapping(flatMapping: FlatMappingAST, outputAST: ObjectExpression) { | ||
let currentOutputPropsAST = outputAST.props; | ||
const currentInputAST = flatMapping.inputExpr; | ||
|
||
const numOutputParts = flatMapping.outputExpr.parts.length; | ||
for (let i = 0; i < numOutputParts; i++) { | ||
if (!currentOutputPropsAST) { | ||
throw new Error(`Failed to process output mapping: ${flatMapping.output}`); | ||
} | ||
const outputPart = flatMapping.outputExpr.parts[i]; | ||
|
||
if (outputPart.type === SyntaxType.SELECTOR && outputPart.prop?.value) { | ||
const key = outputPart.prop.value; | ||
|
||
if (i === numOutputParts - 1) { | ||
currentOutputPropsAST.push({ | ||
type: SyntaxType.OBJECT_PROP_EXPR, | ||
key, | ||
value: currentInputAST, | ||
} as ObjectPropExpression); | ||
break; | ||
} | ||
|
||
const currentOutputPropAST = findOrCreateObjectPropExpression(currentOutputPropsAST, key); | ||
let objectExpr: ObjectExpression = currentOutputPropAST.value as ObjectExpression; | ||
const nextOutputPart = flatMapping.outputExpr.parts[i + 1] as ArrayFilterExpression; | ||
if (nextOutputPart.filter?.type === SyntaxType.ALL_FILTER_EXPR) { | ||
objectExpr = processAllFilter(currentInputAST, currentOutputPropAST); | ||
} else if (nextOutputPart.filter?.type === SyntaxType.ARRAY_INDEX_FILTER_EXPR) { | ||
objectExpr = processArrayIndexFilter( | ||
currentOutputPropAST, | ||
nextOutputPart.filter as IndexFilterExpression, | ||
); | ||
} | ||
currentOutputPropsAST = objectExpr.props; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor processFlatMapping
to improve readability and maintainability by breaking down complex logic into smaller functions.
- function processFlatMapping(flatMapping: FlatMappingAST, outputAST: ObjectExpression) {
+ function processFlatMapping(flatMapping: FlatMappingAST, outputAST: ObjectExpression) {
+ let currentOutputPropsAST = outputAST.props;
+ const currentInputAST = flatMapping.inputExpr;
+ const numOutputParts = flatMapping.outputExpr.parts.length;
+ for (let i = 0; i < numOutputParts; i++) {
+ if (!currentOutputPropsAST) {
+ throw new Error(`Failed to process output mapping: ${flatMapping.output}`);
+ }
+ const outputPart = flatMapping.outputExpr.parts[i];
+ if (outputPart.type === SyntaxType.SELECTOR && outputPart.prop?.value) {
+ const key = outputPart.prop.value;
+ if (i === numOutputParts - 1) {
+ currentOutputPropsAST.push({ type: SyntaxType.OBJECT_PROP_EXPR, key, value: currentInputAST });
+ break;
+ }
+ const currentOutputPropAST = findOrCreateObjectPropExpression(currentOutputPropsAST, key);
+ let objectExpr: ObjectExpression = currentOutputPropAST.value as ObjectExpression;
+ const nextOutputPart = flatMapping.outputExpr.parts[i + 1] as ArrayFilterExpression;
+ if (nextOutputPart.filter?.type === SyntaxType.ALL_FILTER_EXPR) {
+ objectExpr = processAllFilter(currentInputAST, currentOutputPropAST);
+ } else if (nextOutputPart.filter?.type === SyntaxType.ARRAY_INDEX_FILTER_EXPR) {
+ objectExpr = processArrayIndexFilter(currentOutputPropAST, nextOutputPart.filter as IndexFilterExpression);
+ }
+ currentOutputPropsAST = objectExpr.props;
+ }
+ }
+ }
Committable suggestion was skipped due low confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/utils/converter.ts (1 hunks)
Additional context used
Biome
src/utils/converter.ts
[error] 1-13: Some named imports are only used as types.
Additional comments not posted (6)
src/utils/converter.ts (6)
16-20
: TheCreateObjectExpression
function is correctly implemented and adheres to the project's coding standards.
23-37
: ThefindOrCreateObjectPropExpression
function is well-implemented, efficiently handling both finding and creating object properties.
39-56
: TheprocessArrayIndexFilter
function effectively handles different scenarios for array index filtering, ensuring the AST is correctly modified.
58-84
: TheprocessAllFilter
function is correctly implemented, handling the "all filter" with appropriate modifications to the AST.
86-127
: TheprocessFlatMapping
function is well-structured and effectively processes different types of output parts, updating the output AST as needed.
131-139
: TheconvertToObjectMapping
function effectively converts an array of flat mapping ASTs into a single object expression, utilizing well-structured helper functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 13
Outside diff range and nitpick comments (7)
test/scenarios/return/data.ts (1)
Line range hint
1-1
: Use type-only imports for entities used solely as types to clarify their usage and optimize bundling.- import { Scenario } from '../../types'; + import type { Scenario } from '../../types';src/types.ts (1)
Line range hint
108-108
: Specify a more precise type thanany
for these fields to enhance type safety and predictability.Also applies to: 114-114, 127-127
src/translator.ts (2)
53-54
: Initialization ofstandardFunctions
as an empty object seems redundant if it's populated dynamically later.Consider initializing
standardFunctions
directly with the necessary functions if they are known at compile time, or clarify the need for this pattern if it's required for dynamic population.
583-583
: Ensure proper handling of optional chaining and property access.// Use optional chaining appropriately to handle undefined or null values safely. return `${isAssignment ? '' : '?.'}[${escapeStr(expr.prop?.value)}]`;src/parser.ts (3)
Line range hint
538-538
: Specify a different type instead ofany
for error handling inparseConditionalBodyExpr
.- } catch (error: any) { + } catch (error: Error) {
Line range hint
1454-1454
: Usingthis
in a static context can be confusing.Consider refactoring the static methods to avoid using
this
or change the method's static nature if it needs to access instance-specific data.Also applies to: 1460-1460, 1467-1467, 1498-1498
Line range hint
1257-1257
: UseNumber.parseFloat
andNumber.parseInt
instead of the equivalent global functions.- parseFloat(val) + Number.parseFloat(val) - parseInt(val, 10) + Number.parseInt(val, 10)Also applies to: 1263-1263, 1272-1272
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/engine.ts (3 hunks)
- src/parser.ts (18 hunks)
- src/reverse_translator.ts (1 hunks)
- src/translator.ts (15 hunks)
- src/types.ts (10 hunks)
- src/utils/common.ts (1 hunks)
- src/utils/translator.ts (1 hunks)
- test/scenarios/return/data.ts (1 hunks)
Additional context used
Biome
src/utils/translator.ts
[error] 1-1: Some named imports are only used as types.
test/scenarios/return/data.ts
[error] 1-1: All these imports are only used as types.
src/utils/common.ts
[error] 39-39: This function expression can be turned into an arrow function.
[error] 1-7: Some named imports are only used as types.
src/engine.ts
[error] 22-22: Using this in a static context can be confusing.
[error] 29-29: Using this in a static context can be confusing.
[error] 51-51: Using this in a static context can be confusing.
[error] 58-58: Using this in a static context can be confusing.
[error] 70-70: Using this in a static context can be confusing.
[error] 74-74: Using this in a static context can be confusing.
[error] 74-74: Using this in a static context can be confusing.
[error] 6-7: All these imports are only used as types.
[error] 11-11: Don't use 'Function' as a type.
[error] 13-13: Don't use 'Function' as a type.
[error] 20-20: Don't use 'Function' as a type.
[error] 25-25: Don't use 'Function' as a type.
src/types.ts
[error] 108-108: Unexpected any. Specify a different type.
[error] 114-114: Unexpected any. Specify a different type.
[error] 127-127: Unexpected any. Specify a different type.
src/reverse_translator.ts
[error] 1-34: Some named imports are only used as types.
src/translator.ts
[error] 85-85: Unexpected any. Specify a different type.
[error] 230-230: Do not use template literals if interpolation and special-character handling are not needed.
[error] 253-253: Do not use template literals if interpolation and special-character handling are not needed.
[error] 779-779: Do not use template literals if interpolation and special-character handling are not needed.
[error] 857-857: Using this in a static context can be confusing.
[error] 888-888: This type annotation is trivially inferred from its initialization.
[error] 10-42: Some named imports are only used as types.
src/parser.ts
[error] 171-171: Unexpected any. Specify a different type.
[error] 538-538: Unexpected any. Specify a different type.
[error] 1054-1054: This type annotation is trivially inferred from its initialization.
[error] 1454-1454: Using this in a static context can be confusing.
[error] 1460-1460: Using this in a static context can be confusing.
[error] 1467-1467: Using this in a static context can be confusing.
[error] 1498-1498: Using this in a static context can be confusing.
[error] 5-42: Some named imports are only used as types.
[error] 562-562: Do not add then to an object.
[error] 1257-1257: Use Number.parseFloat instead of the equivalent global.
[error] 1263-1263: Use Number.parseInt instead of the equivalent global.
[error] 1272-1272: Use Number.parseFloat instead of the equivalent global.
Additional comments not posted (9)
src/translator.ts (3)
506-516
: Ensure that the handling of standard functions is consistent and error-free.Verification successful
The search results indicate multiple instances of
standardFunctions
insrc/translator.ts
:
- Import statement:
import { binaryOperators, standardFunctions } from './operators';
- Declaration:
private standardFunctions: Record<string, string> = {};
- Usage in a method:
const functions = Object.values(this.standardFunctions);
- Conditional assignment:
if (expr.id && standardFunctions[expr.id]) { this.standardFunctions[expr.id] = standardFunctions[expr.id]; }
These instances suggest that
standardFunctions
is being imported, declared, and used within the class. The conditional assignment within the provided code snippet appears to handle standard functions correctly by checking ifexpr.id
exists instandardFunctions
and then assigning it tothis.standardFunctions
.Based on this analysis, the handling of standard functions seems consistent and error-free.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that standard functions are handled correctly and consistently. rg --type typescript "standardFunctions" src/translator.tsLength of output: 93
Script:
#!/bin/bash # Description: Verify that standard functions are handled correctly and consistently. rg --type ts "standardFunctions" src/translator.tsLength of output: 352
484-490
: Clarify the logic for determining function names to ensure it handles all edge cases correctly.
380-380
: Ensure that the handling of different path types is robust and covers all expected cases.src/parser.ts (6)
229-230
: RefactorparseArrayFilterExpr
to handle object filter expressions more cleanly.
281-301
: RefactorparsePathRoot
to simplify token handling using a switch statement.
Line range hint
1054-1054
: This type annotation is trivially inferred from its initialization.
Line range hint
5-42
: Some named imports are only used as types.
Line range hint
562-562
: Do not addthen
to an object.
153-153
: Specify a more explicit type thanany
for error handling inparseBaseExpr
.- } catch (error: any) { + } catch (error: Error) {Likely invalid or redundant comment.
const functions = Object.values(this.standardFunctions); | ||
if (functions.length > 0) { | ||
code.push(functions.join('').replaceAll(/\s+/g, ' ')); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using replaceAll
for simple string manipulations as it can be less performant than necessary.
// Consider using `replace` with a global regex if you're only removing whitespace.
code.push(functions.join('').replace(/\s+/g, ' '));
const escapedPropName = escapeStr(prop); | ||
code.push(`if(${ctx} && Object.prototype.hasOwnProperty.call(${ctx}, ${escapedPropName})){`); | ||
code.push(`${dest}=${ctx}[${escapedPropName}];`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Properly escape property names to prevent potential security issues or bugs.
// Ensure that property names are escaped correctly to avoid injection vulnerabilities.
const escapedPropName = escapeStr(prop);
code.push(`if(${ctx} && Object.prototype.hasOwnProperty.call(${ctx}, ${escapedPropName})){`);
code.push(`${dest}=${ctx}[${escapedPropName}];`);
@@ -453,7 +460,7 @@ | |||
} | |||
const fnExpr: FunctionExpression = { | |||
type: SyntaxType.FUNCTION_EXPR, | |||
body: CommonUtils.convertToStatementsExpr(...expr.statements), | |||
body: convertToStatementsExpr(...expr.statements), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use direct function calls instead of converting to statements expression for single statements.
// Directly translate the single statement if possible, instead of wrapping it into a function expression.
return this.translateExpr(expr.statements[0], dest, ctx);
@@ -0,0 +1,10 @@ | |||
import { TokenType, Literal } from '../types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use type-only imports for entities used solely as types to clarify their usage and optimize bundling.
- import { TokenType, Literal } from '../types';
+ import type { TokenType, Literal } from '../types';
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
import { TokenType, Literal } from '../types'; | |
import type { TokenType, Literal } from '../types'; |
Tools
Biome
[error] 1-1: Some named imports are only used as types.
@@ -417,7 +424,7 @@ | |||
const result = this.acquireVar(); | |||
code.push(JsonTemplateTranslator.generateAssignmentCode(result, '[]')); | |||
const { prop } = expr; | |||
const propStr = CommonUtils.escapeStr(prop?.value); | |||
const propStr = escapeStr(prop?.value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor to use a more descriptive variable name for better code readability.
- const propStr = escapeStr(prop?.value);
+ const escapedPropertyName = escapeStr(prop?.value);
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
const propStr = escapeStr(prop?.value); | |
const escapedPropertyName = escapeStr(prop?.value); |
@@ -243,7 +250,7 @@ | |||
code.push(`return ${value};`); | |||
this.releaseVars(value); | |||
} | |||
code.push(`return ${ctx};`); | |||
code.push(`return;`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using template literals when not necessary.
// Simplify the return statement without template literals.
code.push('return;');
Tools
Biome
[error] 253-253: Do not use template literals if interpolation and special-character handling are not needed.
@@ -7,7 +7,7 @@ | |||
VARS_PREFIX, | |||
} from './constants'; | |||
import { JsonTemplateTranslatorError } from './errors'; | |||
import { binaryOperators } from './operators'; | |||
import { binaryOperators, standardFunctions } from './operators'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider specifying more precise types instead of any
for better type safety.
// Example: If possible, replace 'any' with specific types like 'Expression' or 'string'.
import { binaryOperators, standardFunctions } from './operators';
Tools
Biome
[error] 10-42: Some named imports are only used as types.
import { EngineOptions, Expression } from './types'; | ||
import { CommonUtils } from './utils'; | ||
import { EngineOptions, Expression, FlatMappingPaths, TemplateInput } from './types'; | ||
import { CreateAsyncFunction, convertToObjectMapping, isExpression } from './utils'; | ||
|
||
export class JsonTemplateEngine { | ||
private readonly fn: Function; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace Function
type with more specific function types to enhance type safety.
- private readonly fn: Function;
- private constructor(fn: Function) {
+ private readonly fn: (...args: any[]) => any;
+ private constructor(fn: (...args: any[]) => any) {
Also applies to: 13-13, 20-20, 25-25
Committable suggestion was skipped due low confidence.
Tools
Biome
[error] 11-11: Don't use 'Function' as a type.
import { | ||
ArrayExpression, | ||
ArrayFilterExpression, | ||
AssignmentExpression, | ||
BinaryExpression, | ||
BlockExpression, | ||
ConditionalExpression, | ||
DefinitionExpression, | ||
EngineOptions, | ||
Expression, | ||
FunctionCallExpression, | ||
FunctionExpression, | ||
IncrementExpression, | ||
IndexFilterExpression, | ||
LambdaArgExpression, | ||
LiteralExpression, | ||
LoopControlExpression, | ||
LoopExpression, | ||
ObjectExpression, | ||
ObjectFilterExpression, | ||
ObjectPropExpression, | ||
PathExpression, | ||
PathOptions, | ||
PathType, | ||
RangeFilterExpression, | ||
ReturnExpression, | ||
SelectorExpression, | ||
SpreadExpression, | ||
StatementsExpression, | ||
SyntaxType, | ||
ThrowExpression, | ||
TokenType, | ||
UnaryExpression, | ||
} from './types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use type-only imports for types that are not used as values to clarify their usage and optimize bundling.
- import {
+ import type {
ArrayExpression,
ArrayFilterExpression,
AssignmentExpression,
BinaryExpression,
BlockExpression,
ConditionalExpression,
DefinitionExpression,
EngineOptions,
Expression,
FunctionCallExpression,
FunctionExpression,
IncrementExpression,
IndexFilterExpression,
LambdaArgExpression,
LiteralExpression,
LoopControlExpression,
LoopExpression,
ObjectExpression,
ObjectFilterExpression,
ObjectPropExpression,
PathExpression,
PathOptions,
PathType,
RangeFilterExpression,
ReturnExpression,
SelectorExpression,
SpreadExpression,
StatementsExpression,
SyntaxType,
ThrowExpression,
TokenType,
UnaryExpression,
} from './types';
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
import { | |
ArrayExpression, | |
ArrayFilterExpression, | |
AssignmentExpression, | |
BinaryExpression, | |
BlockExpression, | |
ConditionalExpression, | |
DefinitionExpression, | |
EngineOptions, | |
Expression, | |
FunctionCallExpression, | |
FunctionExpression, | |
IncrementExpression, | |
IndexFilterExpression, | |
LambdaArgExpression, | |
LiteralExpression, | |
LoopControlExpression, | |
LoopExpression, | |
ObjectExpression, | |
ObjectFilterExpression, | |
ObjectPropExpression, | |
PathExpression, | |
PathOptions, | |
PathType, | |
RangeFilterExpression, | |
ReturnExpression, | |
SelectorExpression, | |
SpreadExpression, | |
StatementsExpression, | |
SyntaxType, | |
ThrowExpression, | |
TokenType, | |
UnaryExpression, | |
} from './types'; | |
import type { | |
ArrayExpression, | |
ArrayFilterExpression, | |
AssignmentExpression, | |
BinaryExpression, | |
BlockExpression, | |
ConditionalExpression, | |
DefinitionExpression, | |
EngineOptions, | |
Expression, | |
FunctionCallExpression, | |
FunctionExpression, | |
IncrementExpression, | |
IndexFilterExpression, | |
LambdaArgExpression, | |
LiteralExpression, | |
LoopControlExpression, | |
LoopExpression, | |
ObjectExpression, | |
ObjectFilterExpression, | |
ObjectPropExpression, | |
PathExpression, | |
PathOptions, | |
PathType, | |
RangeFilterExpression, | |
ReturnExpression, | |
SelectorExpression, | |
SpreadExpression, | |
StatementsExpression, | |
SyntaxType, | |
ThrowExpression, | |
TokenType, | |
UnaryExpression, | |
} from './types'; |
Tools
Biome
[error] 1-34: Some named imports are only used as types.
import { convertToStatementsExpr, escapeStr } from './utils/common'; | ||
import { translateLiteral } from './utils/translator'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The imports escapeStr
and translateLiteral
are indeed used in the src/translator.ts
file. However, the import convertToStatementsExpr
is not used.
escapeStr
is used in multiple lines.translateLiteral
is used in thetranslateLiteralExpr
method.convertToStatementsExpr
is not found in the usage context.
Please remove the unused import convertToStatementsExpr
from src/translator.ts
.
Analysis chain
Ensure that utility functions are used efficiently and check if all imports are necessary.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for unused imports in src/translator.ts
rg --type typescript "import" src/translator.ts | rg --type typescript -v "escapeStr|translateLiteral"
Length of output: 172
Script:
#!/bin/bash
# Description: Check for unused imports in src/translator.ts by searching for their usage
rg 'escapeStr|translateLiteral' src/translator.ts
Length of output: 586
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/utils/converter.ts (1 hunks)
- test/scenarios/mappings/all_features.json (1 hunks)
- test/scenarios/mappings/data.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- test/scenarios/mappings/all_features.json
- test/scenarios/mappings/data.ts
Additional context used
Biome
src/utils/converter.ts
[error] 1-12: Some named imports are only used as types.
Additional comments not posted (7)
src/utils/converter.ts (7)
15-20
: TheCreateObjectExpression
function is correctly implemented.
22-36
: ThefindOrCreateObjectPropExpression
function is correctly implemented.
38-55
: TheprocessArrayIndexFilter
function is correctly implemented.
57-83
: TheprocessAllFilter
function is correctly implemented.
85-99
: ThehandleNextPart
function is correctly implemented.
102-134
: TheprocessFlatMappingPart
function is correctly implemented.
139-150
: TheconvertToObjectMapping
function is correctly implemented.
import { | ||
SyntaxType, | ||
PathExpression, | ||
ObjectPropExpression, | ||
ArrayExpression, | ||
ObjectExpression, | ||
FlatMappingAST, | ||
Expression, | ||
IndexFilterExpression, | ||
BlockExpression, | ||
} from '../types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use type-only imports for types that are not used as values.
- import {
+ import type {
SyntaxType,
PathExpression,
ObjectPropExpression,
ArrayExpression,
ObjectExpression,
FlatMappingAST,
Expression,
IndexFilterExpression,
BlockExpression,
} from '../types';
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
import { | |
SyntaxType, | |
PathExpression, | |
ObjectPropExpression, | |
ArrayExpression, | |
ObjectExpression, | |
FlatMappingAST, | |
Expression, | |
IndexFilterExpression, | |
BlockExpression, | |
} from '../types'; | |
import type { | |
SyntaxType, | |
PathExpression, | |
ObjectPropExpression, | |
ArrayExpression, | |
ObjectExpression, | |
FlatMappingAST, | |
Expression, | |
IndexFilterExpression, | |
BlockExpression, | |
} from '../types'; |
Tools
Biome
[error] 1-12: Some named imports are only used as types.
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/engine.ts (3 hunks)
- test/types.ts (1 hunks)
- test/utils/scenario.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- test/types.ts
Additional context used
Biome
test/utils/scenario.ts
[error] 6-36: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
[error] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 28-28: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
src/engine.ts
[error] 22-22: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 29-29: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 51-51: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 58-58: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 74-74: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 74-74: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 83-83: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 83-83: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 11-11: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
[error] 13-13: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
[error] 20-20: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
[error] 25-25: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
Additional comments not posted (7)
test/utils/scenario.ts (1)
24-24
: Replacethis
withScenarioUtils
in static methods to avoid confusion.Tools
Biome
[error] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.src/engine.ts (6)
25-25
: Replacethis
withJsonTemplateEngine
andFunction
type with a more specific function type in static methods to enhance clarity and type safety.Also applies to: 13-13
Tools
Biome
[error] 25-25: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
33-35
: LGTM! Good use of theJsonTemplateTranslator
for expression translation.
51-51
: Replacethis
withJsonTemplateEngine
in static methods to avoid confusion.Tools
Biome
[error] 51-51: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
58-58
: Replacethis
withJsonTemplateEngine
in static methods to avoid confusion.Tools
Biome
[error] 58-58: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
70-70
: Replacethis
withJsonTemplateEngine
in static methods to avoid confusion.Tools
Biome
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
73-73
: Replacethis
withJsonTemplateEngine
in static methods to avoid confusion.
|
||
static createTemplateEngine(scenarioDir: string, scenario: Scenario): JsonTemplateEngine { | ||
this.initializeScenario(scenarioDir, scenario); | ||
return JsonTemplateEngine.create(scenario.template as string, scenario.options); | ||
} | ||
|
||
static evaluateScenario(templateEngine: JsonTemplateEngine, scenario: Scenario): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify a more precise type than any
for the return type of evaluateScenario
.
- static evaluateScenario(templateEngine: JsonTemplateEngine, scenario: Scenario): any {
+ static evaluateScenario(templateEngine: JsonTemplateEngine, scenario: Scenario): ReturnType<JsonTemplateEngine['evaluate']> {
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
static evaluateScenario(templateEngine: JsonTemplateEngine, scenario: Scenario): any { | |
static evaluateScenario(templateEngine: JsonTemplateEngine, scenario: Scenario): ReturnType<JsonTemplateEngine['evaluate']> { |
Tools
Biome
[error] 28-28: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
private static initializeScenario(scenarioDir: string, scenario: Scenario) { | ||
scenario.options = scenario.options || {}; | ||
scenario.options.defaultPathType = scenario.options.defaultPathType || PathType.SIMPLE; | ||
return JsonTemplateEngine.create(template, scenario.options); | ||
const templatePath = join(scenarioDir, Scenario.getTemplatePath(scenario)); | ||
let template: string = readFileSync(templatePath, 'utf-8'); | ||
if (scenario.containsMappings) { | ||
template = JsonTemplateEngine.convertMappingsToTemplate( | ||
JSON.parse(template) as FlatMappingPaths[], | ||
); | ||
} | ||
scenario.template = JsonTemplateEngine.reverseTranslate( | ||
JsonTemplateEngine.parse(template, scenario.options), | ||
scenario.options, | ||
); | ||
} | ||
|
||
static createTemplateEngine(scenarioDir: string, scenario: Scenario): JsonTemplateEngine { | ||
this.initializeScenario(scenarioDir, scenario); | ||
return JsonTemplateEngine.create(scenario.template as string, scenario.options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor ScenarioUtils
to use simple functions instead of static methods.
- export class ScenarioUtils {
- private static initializeScenario(scenarioDir: string, scenario: Scenario) {
+ export function initializeScenario(scenarioDir: string, scenario: Scenario) {
Continue this pattern for other methods in the class.
Committable suggestion was skipped due to low confidence.
Tools
Biome
[error] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
} | ||
|
||
static createTemplateEngine(scenarioDir: string, scenario: Scenario): JsonTemplateEngine { | ||
this.initializeScenario(scenarioDir, scenario); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace this
with ScenarioUtils
in static methods to avoid confusion.
- this.initializeScenario(scenarioDir, scenario);
+ ScenarioUtils.initializeScenario(scenarioDir, scenario);
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
this.initializeScenario(scenarioDir, scenario); | |
ScenarioUtils.initializeScenario(scenarioDir, scenario); |
Tools
Biome
[error] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
@@ -31,40 +22,68 @@ export class JsonTemplateEngine { | |||
return Function(DATA_PARAM_KEY, BINDINGS_PARAM_KEY, this.translate(templateOrExpr, options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace this
with JsonTemplateEngine
and Function
type with a more specific function type in static methods to enhance clarity and type safety.
- return Function(DATA_PARAM_KEY, BINDINGS_PARAM_KEY, this.translate(templateOrExpr, options));
+ return (data: typeof DATA_PARAM_KEY, bindings: typeof BINDINGS_PARAM_KEY) => JsonTemplateEngine.translate(templateOrExpr, options);
Also applies to: 11-11
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
return Function(DATA_PARAM_KEY, BINDINGS_PARAM_KEY, this.translate(templateOrExpr, options)); | |
return (data: typeof DATA_PARAM_KEY, bindings: typeof BINDINGS_PARAM_KEY) => JsonTemplateEngine.translate(templateOrExpr, options); |
Tools
Biome
[error] 22-22: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
private static parseMappingPaths( | ||
mappings: FlatMappingPaths[], | ||
options?: EngineOptions, | ||
): Expression { | ||
const flatMappingAST = mappings.map((mapping) => ({ | ||
...mapping, | ||
inputExpr: JsonTemplateEngine.parse(mapping.input, options).statements[0], | ||
outputExpr: JsonTemplateEngine.parse(mapping.output, options).statements[0], | ||
})); | ||
return convertToObjectMapping(flatMappingAST); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider refactoring parseMappingPaths
to improve readability by breaking down complex expressions.
+ const flatMappingAST = mappings.map((mapping) => {
+ const inputExpr = JsonTemplateEngine.parse(mapping.input, options).statements[0];
+ const outputExpr = JsonTemplateEngine.parse(mapping.output, options).statements[0];
+ return { ...mapping, inputExpr, outputExpr };
+ });
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
private static parseMappingPaths( | |
mappings: FlatMappingPaths[], | |
options?: EngineOptions, | |
): Expression { | |
const flatMappingAST = mappings.map((mapping) => ({ | |
...mapping, | |
inputExpr: JsonTemplateEngine.parse(mapping.input, options).statements[0], | |
outputExpr: JsonTemplateEngine.parse(mapping.output, options).statements[0], | |
})); | |
return convertToObjectMapping(flatMappingAST); | |
} | |
private static parseMappingPaths( | |
mappings: FlatMappingPaths[], | |
options?: EngineOptions, | |
): Expression { | |
const flatMappingAST = mappings.map((mapping) => { | |
const inputExpr = JsonTemplateEngine.parse(mapping.input, options).statements[0]; | |
const outputExpr = JsonTemplateEngine.parse(mapping.output, options).statements[0]; | |
return { ...mapping, inputExpr, outputExpr }; | |
}); | |
return convertToObjectMapping(flatMappingAST); | |
} |
What are the changes introduced in this PR?
Write a brief explainer on your code changes.
add support for json path syntax using new path type ~j
What is the related Linear task?
Resolves INT-2160
Please explain the objectives of your changes below
Put down any required details on the broader aspect of your changes. If there are any dependent changes, mandatorily mention them here
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new generic utility introduced or modified. Please explain the changes.
N/A
Any technical or performance related pointers to consider with the change?
N/A
@coderabbitai review
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
Is the PR limited to 10 file changes?
Is the PR limited to one linear task?
Are relevant unit and component test-cases added?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
New Features
sum
,max
,min
,avg
,length
,stddev
,first
,last
,index
, andkeys
.Documentation
Tests